feat(groq): map context-length errors to ContextOverflowError#37676
feat(groq): map context-length errors to ContextOverflowError#37676Aditya Singh (adityasingh2400) wants to merge 1 commit into
ContextOverflowError#37676Conversation
Groq's context-overflow errors were raised as plain `groq.BadRequestError`, so downstream code could not catch them uniformly through the shared `langchain_core.exceptions.ContextOverflowError` the way it can for the Anthropic, OpenAI, and Fireworks integrations. This adds a `GroqContextOverflowError` subclass plus a promoter that detects Groq's `context_length_exceeded` signal, mirroring the existing partner integrations, and wires it into the sync and async generate and stream paths. Unit tests cover the promotion across all four paths plus the non-context case. Addresses langchain-ai#37533
This comment has been minimized.
This comment has been minimized.
ContextOverflowError
Kevin Kakolla (torontodeveloper)
left a comment
There was a problem hiding this comment.
I am not great fan o reviewing unit test files since they won't have any impact on production feature per se, I will leave it upto u as long as they are testing what is intended to be for
| """`BadRequestError` raised when input exceeds Groq's context limit.""" | ||
|
|
||
|
|
||
| def _handle_groq_invalid_request(e: groq.BadRequestError) -> NoReturn: |
There was a problem hiding this comment.
just curious why this method is made private to this class with '_' to start with, is it so others can't invoke this method and hence made private?
There was a problem hiding this comment.
Yes, the leading underscore marks it as module-private, it is an internal helper that the two BadRequestError handlers call and is not part of the public API. That matches the existing convention in this file, where the other helpers are written the same way, for example _get_default_model_profile, _create_chat_result, and _convert_dict_to_message. Keeping it private also lets the implementation change later without it being a public surface anyone depends on.
| if ( | ||
| getattr(e, "code", None) == "context_length_exceeded" | ||
| or "context_length_exceeded" in str(e) | ||
| or "reduce the length" in str(e) | ||
| ): | ||
| raise GroqContextOverflowError(str(e), response=e.response, body=e.body) from e | ||
| raise e |
There was a problem hiding this comment.
I notice lot of developers use single char in open source, having coming other languages like C++, Java, I wouldnt encourage usage of single syllabel for variable names, As u know as Zen of Python, readibility counts, i would use atleast ex to make it more explict that 'e' meant exception
There was a problem hiding this comment.
Fair point in general on readability. I used e here to stay consistent with the rest of this file and the sibling partner integrations I mirrored, the existing except Exception as e at the bottom of this module uses the same name. The scope is two lines and the type is right there in the signature (e: groq.BadRequestError), so the meaning stays clear. Happy to rename to exc if the maintainers prefer that, the file also has one except ImportError as exc, so there is a mild mix already.
There was a problem hiding this comment.
I am Ok eitherway, as it's not a major issue and one of hardest things to solve in computer science is Naming and the other one is caching so we can leave it there.
But a quote came to my mind "A foolish consistency is the hobgoblin of not great minds,"
| response = await self.async_client.create(messages=message_dicts, **params) | ||
| except groq.BadRequestError as e: | ||
| _handle_groq_invalid_request(e) | ||
| return self._create_chat_result(response, params) |
There was a problem hiding this comment.
just curious, do we reach line 679 of return statment if there is exception thrown by async_client.create() or do we need finally?
There was a problem hiding this comment.
Good question. We do not reach the return on a context-overflow error. _handle_groq_invalid_request is annotated -> NoReturn and always raises: it either raises the promoted GroqContextOverflowError or re-raises the original BadRequestError. So when create() throws a BadRequestError, the except calls the helper, the helper raises, and control never falls through to _create_chat_result. No finally is needed, and we deliberately do not want one here because there is no resource to clean up, response is simply never assigned on the error path.
| _handle_groq_invalid_request(e) | ||
| default_chunk_class: type[BaseMessageChunk] = AIMessageChunk | ||
| for chunk in self.client.create(messages=message_dicts, **params): | ||
| for chunk in stream: |
There was a problem hiding this comment.
so what happens if client.create() throws exception what would be stream here and probably loop wont be executed right?
There was a problem hiding this comment.
Right, the loop does not run on an exception. self.client.create(...) is called inside the try, and if it raises BadRequestError the except calls _handle_groq_invalid_request, which is NoReturn and always raises. So execution leaves the function before stream is ever bound, and for chunk in stream is never reached. stream only gets a value when create() returned normally, so there is no risk of iterating an unbound or partial stream.
| assert expected == _format_message_content(content) | ||
|
|
||
|
|
||
| def _bad_request_error(body: dict[str, Any], status_code: int = 400) -> Exception: |
There was a problem hiding this comment.
does this has to be private method? looks like it as this is not unit test case but helper method for remaining test cases?
There was a problem hiding this comment.
Right, _bad_request_error is a fixture builder, not a test case, so the underscore keeps pytest from collecting it as a test and signals it is a local helper for the tests below. pytest discovers test_-prefixed functions, so naming it _bad_request_error avoids it being mistaken for one while still being importable within the module.
| _CONTEXT_OVERFLOW_BODY = { | ||
| "error": { | ||
| "message": "Please reduce the length of the messages or completion.", | ||
| "type": "invalid_request_error", | ||
| "param": "messages", | ||
| "code": "context_length_exceeded", | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not sure if this has to be private variable? Also, it's in upper case, I would assume if this is some kinda constant? as per convention, not necessarily syntax error though
There was a problem hiding this comment.
It is a module-level test constant, so the upper-case name follows the PEP 8 convention for constants, and the underscore keeps it scoped to this test module rather than exported. It is fixed input data reused across the promotion tests, so treating it as a named constant rather than rebuilding it in each test keeps the cases readable. Happy to adjust the name if you would rather it not be a constant.
There was a problem hiding this comment.
I am Ok if u are convinced the way it is. Besides, naming is not major issue, it's more of readibility for u and other guys like myself
Fixes #37533
langchain-coredefinesContextOverflowErrorso that application code can catch an over-long prompt the same way regardless of which provider raised it. The Anthropic, OpenAI, and Fireworks integrations already promote their provider-specific context-length errors to a subclass of it, butlangchain-groqdid not: a context overflow there surfaced as a plaingroq.BadRequestError, so anyone relying on the shared exception had to special-case Groq.This closes that gap for Groq. It adds a
GroqContextOverflowError(a subclass of bothgroq.BadRequestErrorandContextOverflowError) and a small promoter,_handle_groq_invalid_request, wired into the sync and asyncgenerateandstreampaths. Because Groq's SDK mirrors OpenAI's, the implementation follows the same shape as the existing partners, and the promoted error keeps the originalresponseandbodyso existing catchers that inspect.response.status_codekeep working. Anything that already catchesgroq.BadRequestErroris unaffected, since the new class is still aBadRequestError.One detail worth a reviewer's eye: Groq returns the overflow as a 400 whose JSON body carries
"code": "context_length_exceeded", but the SDK'sBadRequestErrordoes not expose that code as an attribute. The SDK does fold the full JSON body into the error message, so detection primarily matchescontext_length_exceededagainst the stringified error, withreduce the lengthfrom the message as a secondary signal and an attribute check kept as defensive cover in case a future SDK adds.code. The unit tests construct the error exactly as the SDK does for a 4xx response and assert promotion across all four call paths, that an unrelatedBadRequestErroris left untouched, and thatresponse/bodyare preserved.I scoped this to Groq and left Mistral as a follow-up: Mistral surfaces errors as raw
httpx.HTTPStatusErrorrather than a typed SDK error, and I could not verify its exact context-overflow signal (status code plus bodycode/message) against an authoritative source well enough to assert it in a unit test without live API access, so I would rather not guess at the shape.